[suggestion] EvaluateEfSearchRange relies entirely on side effects — the winning ef_search value is not returned

The function evaluates ef_search candidates via the evaluate callback and narrows the range through exponential probing + binary search, but it returns void. The caller (EvaluateCandidates) has no programmatic way to determine which ef_search value was the minimal passing one — it must scan all recorded trials to find it. This works because SelectResult does exactly that scan, but the contract between EvaluateEfSearchRange and its caller is implicit.

Consider returning the optimal ef_search value (or std::nullopt if none found) to make the contract explicit and allow the caller to tag the winning trial directly.

[suggestion] expand_range floating-point loop uses uint64_t counter that can overflow before reaching stop

The loop for (uint64_t i = 0;; ++i) computes current = start + static_cast<double>(i) * step. For very small steps (e.g. step=1e-9) over a moderate range, i can overflow uint64_t before current reaches stop. The max_trials check in generate_candidates provides indirect protection, but the overflow would happen inside expand_range before generate_candidates ever sees the emitted values.

Consider adding an explicit iteration cap inside expand_range (e.g. if (i > some_reasonable_limit) throw ...) or using the max_trials value as a direct bound passed into the expand functions.

[suggestion] SelectResult uses int64_t for vector indices — type mismatch with size_t

best and best_effort are declared as int64_t (lines 736-737) but are used to index into trials (a JsonType array backed by std::vector). While max_trials is capped at 100000, the signed/unsigned mismatch could produce compiler warnings and is semantically incorrect — a negative index would be a bug but is silently representable.

Consider using size_t or JsonType::size_type for these index variables, with std::numeric_limits<size_t>::max() as the sentinel instead of -1.

[suggestion] run_tuning_locked failure lambda captures keep_intermediate by reference before it is assigned

The failure lambda at line 863 captures keep_intermediate by reference, but keep_intermediate is not assigned until line 889 (from context.keep_intermediate). If the parser at line 887 throws, keep_intermediate is still false (its default), which is the correct cleanup behavior. However, the temporal coupling between the lambda definition and the assignment is fragile — reordering the code could silently break cleanup.

Consider moving the keep_intermediate assignment before the lambda definition, or capturing it by value after assignment.

[note] FromDatasets validates every ground truth ID against the base dataset (O(N*K) lookup)

The validation loop at lines 311-315 calls GetOneTrainById() for every ground truth ID, which does an unordered_map::find(). For large datasets this is O(N*K) expected time. This runs once per TuneIndex/TuneSearch call. The author has acknowledged this and deferred cross-request caching until profiling justifies it — which is reasonable for V1.

No action needed now, but worth keeping in mind if tuning performance on million-scale datasets becomes a bottleneck.

[note] EvaluateEfSearchRange binary search correctly avoids evaluating stop unnecessarily

The exponential probing doubles high from low until recall >= target or high == stop. When high reaches stop and still fails, the function returns early (line 169-171). When high passes before reaching stop, the binary search narrows between low (last failing) and high (first passing). The stop value is only evaluated if all earlier probes fail — which is the correct behavior for proving the range infeasible. The implementation matches the documented design.

[suggestion] search_seconds metric uses wall-clock time

[suggestion] search_seconds metric uses wall-clock time which conflates parallelism with per-query cost

In autotune_evaluation.cpp, search_metrics(raw, elapsed(measured_start)) records search_seconds as the wall-clock duration of the entire search evaluation batch. When concurrency > 1, this wall-clock time shrinks with more threads even though the total CPU work is the same. This makes search_seconds and the derived build_and_search_seconds incomparable across different concurrency settings.

Consider dividing by concurrency to get an amortized per-query cost, or use the latency_avg_ms metric (which already measures per-query time) as the primary search-cost signal.

[suggestion] build_metrics silently ignores missing duration(s) — consider logging a warning

build_metrics() at line 60 calls number(raw, "duration(s)") which returns std::nullopt when the field is absent or non-numeric. This silently produces a MetricMap without build_seconds. While the current BuildEvalCase always emits duration(s) via the DurationMonitor, a future change to the monitor pipeline could silently break the metric without any diagnostic.

Consider logging a warning when duration(s) is missing from the raw build result, so that regressions in the eval pipeline are immediately visible in AutoTune output.

[suggestion] SelectResult best-effort ranking uses unscaled violation score that favors large-magnitude metrics

In SelectResult() at line 770-778, the violation score is computed as abs(actual - expected) / max(expected, 1e-12). When multiple constraints are violated, the total score is the sum of per-metric ratios. This means a metric with large absolute values (e.g. qps at 10000 vs expected 5000 → ratio 1.0) dominates over a metric with small values (e.g. recall_at_k at 0.8 vs expected 0.9 → ratio 0.11). The best-effort selection may prefer a candidate that barely misses a high-magnitude constraint over one that severely violates a [0,1]-range constraint.

Consider normalizing each violation ratio to [0,1] (e.g. via ratio / (1 + ratio)) or weighting constraints equally so that the ranking is not magnitude-dependent.

[suggestion] finalize_artifacts removes parent directories unconditionally, which may collide with concurrent runs

In finalize_artifacts() → update_artifact() at line 978-982, when an artifact file is removed, the code also attempts to remove its parent directory and grandparent directory via std::filesystem::remove(). If two concurrent TuneIndex calls (serialized by run_mutex()) share overlapping workspace paths, the second call may find its artifact directories already removed by the first call cleanup. While the global mutex prevents true concurrency, if the mutex is ever relaxed (as suggested in another review), this cleanup logic would be unsafe.

Consider using a unique per-run subdirectory (already done via new_run_name()) and only removing the run-specific directory, not walking up the tree.

[suggestion] ScopedOpenMpThreads in evaluator.cpp snapshots but does not set thread count — differs from test-fixture version

The ScopedOpenMpThreads in evaluator.cpp:29-36 only saves omp_get_max_threads() in its constructor without calling omp_set_num_threads(). It relies on SearchEvalCase::do_knn_search() to set the thread count later. This is different from the ScopedOpenMpThreads in autotune_test.cpp:86-93 which accepts a thread count and sets it immediately.

If EvaluateSearch is ever extended to support non-knn search modes (range search, filter search), those code paths may not call omp_set_num_threads(), leaving the thread count at whatever the caller left it at. Consider either: (a) having the evaluator version also accept and set the thread count, or (b) documenting that the evaluator version is intentionally passive and the search case is responsible for thread configuration.

[suggestion] do_knn_search fallback statistics pass re-executes all queries without SearchFailure protection

In do_knn_search() at line 298-314, when statistics_collected is false after the monitor loop (i.e. no non-latency monitor was configured), a fallback pass executes queries solely to collect statistics. This fallback pass uses SearchFailure for error propagation, which is good. However, it does not check search_failure.Failed() inside the parallel loop body before calling prepare_query() and KnnSearch(), unlike the main monitor loop at line 248. If a prior query in the same batch failed, subsequent iterations still execute — they will also likely fail, wasting work.

Consider adding the if (search_failure.Failed()) { continue; } guard to the fallback pass for consistency with the main loop.

[suggestion] expand_range floating-point loop may produce excessive iterations for tiny step values

The floating-point range expansion at line 117-136 uses for (uint64_t i = 0;; ++i) with a current == previous duplicate check as the only non-convergence guard. For very small step values (e.g. step=1e-15 over [0, 1]), this could produce up to 1e15 iterations before the duplicate check triggers. While max_trials in the caller provides a safety net, the check happens after candidate generation completes.

Consider adding an explicit iteration cap inside expand_range itself (e.g. if (i > 10000000) throw ...) to fail fast rather than relying on the downstream max_trials check which only triggers after all ranges have been fully expanded.

[suggestion] search_seconds records wall-clock time, which conflates parallelism with per-query cost

At line 85, search_seconds = elapsed(measured_start) captures the total wall-clock duration of the entire search evaluation loop. When OpenMP parallelism is active, this measures real time (e.g. 0.5s for 8-thread execution) rather than aggregate CPU time. The metric is then used as an objective for candidate comparison, meaning a candidate evaluated with more threads will appear faster even if its per-query latency is identical.

If the intent is to compare per-query algorithmic cost independent of parallelism, consider dividing by the number of queries or using omp_get_wtime() accumulation per query. If wall-clock throughput is the intended metric, document this clearly in the metric name (e.g. wall_clock_seconds) and the API doc.

[suggestion] build_metrics silently ignores missing duration(s), producing a zero-filled record

At line 60, build_metrics reads duration(s) from the eval result JSON. If the key is absent (e.g. due to a future eval config change or a monitor that skips duration), json.value(key, 0.0) silently defaults to 0.0. The caller then treats this as a valid build time of zero seconds, which could rank a broken or incomplete build above properly measured candidates.

Consider either: (a) making duration(s) required and throwing if absent, or (b) using NaN / std::optional as the default so downstream scoring can detect and penalize missing data.

[suggestion] SelectResult best-effort violation scoring favors large-magnitude metrics

At line 770, when no candidate satisfies all constraints, the fallback scoring computes sum += (metric_value - threshold) / threshold. This normalizes each violation by its threshold, which is correct. However, the score is an unweighted sum across all metrics. If one metric (e.g. search_seconds) has a threshold of 0.01 and another (e.g. memory_usage) has a threshold of 1e9, a 10% violation of memory adds 0.1 to the score while a 10x violation of search_seconds adds only 9.0 — making the sum dominated by the metric with the smallest relative violation range.

Consider using a weighted sum or normalizing to a common scale, or documenting that constraints with small absolute thresholds inherently carry more weight in the best-effort selection.

[suggestion] finalize_artifacts removes parent directories unconditionally, risking unintended deletion

At line 978, std::filesystem::remove_all(workspace) deletes the entire workspace directory tree. If the workspace path is misconfigured (e.g. set to "." or a shared directory), this could delete files outside the intended tuning artifacts. The current validation in make_context checks that the workspace is absolute and non-empty, but does not verify it is a dedicated tuning directory.

Consider adding a sentinel file check (e.g. require a .autotune_workspace marker) before deletion, or restricting cleanup to only the known subdirectories created during tuning.

[suggestion] ScopedOpenMpThreads in evaluator.cpp diverges from the version in autotune_test.cpp

evaluator.cpp:29-36 defines ScopedOpenMpThreads with a default constructor that snapshots omp_get_max_threads() but does NOT call omp_set_num_threads(). In contrast, autotune_test.cpp:86-93 defines a version whose constructor accepts a thread count and calls omp_set_num_threads(value).

The evaluator version is used in EvaluateSearch() at line 97, which means the search evaluation inherits whatever thread count was set by the caller rather than explicitly controlling it. The test version is used to verify that omp_set_num_threads(3) is properly restored.

If both classes are meant to represent the same RAII guard, consider consolidating them into a single definition (e.g. in a shared header) to avoid the two implementations drifting further apart.

[suggestion] do_knn_search fallback statistics pass missing SearchFailure guard

At line 300, the fallback statistics loop iterates over all queries a second time to collect per-query metrics. This pass runs after the main search loop but does not check SearchFailure::Failed() before accessing result data. If a query in the main loop recorded a failure via SearchFailure::Record(), the corresponding result entry may be incomplete or invalid, and the statistics pass could read uninitialized or partial data.

Consider either: (a) checking SearchFailure::Failed() before the statistics pass and skipping it entirely on failure, or (b) guarding each per-query statistic access with a per-query validity check.